Add technical specification for user collections factory#113
Add technical specification for user collections factory#113Douglasacost wants to merge 32 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial technical specification and documentation index for an operator-triggered “User Collections” NFT factory (UUPS-upgradeable factory deploying immutable EIP-1167 ERC-721/ERC-1155 clones), including roles, interfaces, flows, storage notes, security model, testing plan, and ops/deployment guidance.
Changes:
- Introduce a full technical specification for the user collections factory + clone templates.
- Add a documentation README that indexes the specification.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/collections/doc/spec/user-collections-specification.md | New end-to-end technical spec describing architecture, interfaces, security, testing, and operations for the collections factory/clones. |
| src/collections/doc/README.md | Documentation landing page linking to the technical specification. |
LCOV of commit
|
Specifies an operator-triggered NFT collection factory: users pay in fiat off-chain, a trusted backend deploys a fully-isolated EIP-1167 clone (ERC-721 or ERC-1155) on the user's behalf. Factory is UUPS-upgradeable; clones are immutable per release. Both creator and operator hold MINTER_ROLE on each clone; per-collection metadata (baseURI/uri + contractURI) and royalties are owner-mutable until locked one-way. On-chain externalId map prevents duplicate creations from the off-chain ledger. Operational sections describe the actual zkSync flow: deployment via a Forge script + ops/ shell wrapper mirroring ops/deploy_swarm_contracts_zksync.sh, source-code verification via ops/verify_zksync_contracts.py against the zkSync verifier API, and a manual pre-upgrade storage-layout check matching src/swarms/doc/upgradeable-contracts.md. CI today runs forge test on all packages and enforces 95% coverage on swarms only; collection-coverage and storage-layout-diff CI jobs are recorded as deferred items.
These three technical terms are used by the new src/collections/doc/spec/user-collections-specification.md but were not in the project's cspell ignoreWords list. EXTCODEHASH is an EVM opcode (sits next to the existing EXTCODECOPY entry); autonumber is a Mermaid sequence-diagram keyword; dedup is a common abbreviation.
c869607 to
93e660b
Compare
Updates the user collections specification with nine targeted refinements from design review and wires the collections coverage gate in CI: - Operator MINTER_ROLE auto-granted by the factory on every clone (no longer a backend convention). - Royalty cap intentionally absent: full creator autonomy up to OZ's ERC-2981 100% bound; recorded as a design decision. - ERC-721 mintBatch returns the contiguous tokenIds array for backend reconciliation without log parsing. - Factory setImplementation* and initialize reject zero addresses and non-contracts (NotAContract error). - Clone storage layout: metadataLocked and royaltiesLocked share one slot; pre-upgrade diff must verify byte offsets, not just slot indices. - ERC-1155 mintBatch stays single-recipient (OZ alignment); multi- recipient deferred with explicit trigger condition. - Bytecode-permanence invariants: implementations contain no SELFDESTRUCT, no caller-controlled delegatecall, deployed via CREATE only (no CREATE2 redeploy path). - Coverage CI gate wired now; storage-layout diff CI deferred to the V2 upgrade PR with a baseline-JSON convention for v1. - UUPS upgrade tests tightened: slot change verification, OPERATOR_ROLE rejection, storage preservation via V2 mock, non-UUPS revert. Adds a new design-decision row codifying OZ alignment as a governing principle for src/collections/.
…terfaces First contract draft of the user collections system. Mirrors the spec at src/collections/doc/spec/user-collections-specification.md. - ICollectionFactory, IUserCollection721, IUserCollection1155, and shared CollectionTypes (Standard enum, CreateParams721, CreateParams1155). - UserCollection721: ERC-721 + URIStorage + Burnable + ERC-2981 + AccessControl. Owner/minter roles, mint and mintBatch (returns minted tokenIds), opt-in metadata/royalties locks, packed lock booleans, __gap reserved, _disableInitializers in constructor. - UserCollection1155: ERC-1155 + Supply + Burnable + ERC-2981 + AccessControl. Single-recipient mintBatch matching OZ's _mintBatch shape, same lock semantics. - CollectionFactory: UUPS-upgradeable, OPERATOR_ROLE-gated creation, atomic Clones.clone + initialize + externalId mapping write, setImplementation* with ZeroAddress / NotAContract validation, _authorizeUpgrade gated to DEFAULT_ADMIN_ROLE. Operator MINTER_ROLE auto-grant on every clone (passes msg.sender to clone.initialize). - .cspell.json: allow runbook, selfdestruct, SELFDESTRUCT, proxiable (referenced in spec/contracts). Tests, deployment scripts, and the V2 mock fixtures will land in follow-up commits.
…inits Test coverage for the user collections system: - UserCollection721.t.sol (24 tests): initialize wiring, mint / mintBatch with returned tokenIds asserted contiguous and matching Transfer events, lock semantics on baseURI/contractURI/royalties, role-admin grant + revoke, ERC-2981 non-zero and zero (clear) paths, supportsInterface for ERC-165/721/Metadata/2981/AccessControl, ERC721Burnable scenarios, bytecode-permanence opcode walker (skips PUSH1..PUSH32 immediates). - UserCollection1155.t.sol (18 tests): analogous coverage adapted to ERC-1155 mechanics, including ERC1155Supply / Burnable behavior. - CollectionFactory.t.sol (21 tests): atomic clone+init, externalId reverts, operator auto-grant invariant, setImplementation* validation, and the four UUPS assertions from spec §8.1 — slot change via vm.load, OPERATOR-only rejection, storage preservation through V2 mock, non-UUPS revert via ERC1967InvalidImplementation. - Collections.integration.t.sol: end-to-end happy path from §8.4. - mocks/CollectionFactoryV2Mock.sol + NonUUPSImplementationMock.sol. Drops empty `__<Mixin>_init` calls (ERC721URIStorage, Burnable, ERC2981, AccessControl, ERC1155Supply, etc.) which are no-op shims in OZ v5.6.1 kept only as forward-compat aliases. Comment notes to re-add them on OZ upgrade. Functional inits (`__ERC721_init`, `__ERC1155_init`) and `_disableInitializers()` retained. Coverage: 157/162 = 96.91% lines (CollectionFactory 97.78%, UserCollection1155 96.08%, UserCollection721 96.97%). Clears the 95% threshold enforced by .github/workflows/checks.yml.
…yout baselines
Adds the deployment and upgrade tooling for the user collections system,
mirroring the swarms pattern.
- script/DeployCollectionFactoryZkSync.s.sol: Forge script that deploys
UserCollection721 and UserCollection1155 implementations (CREATE only),
the CollectionFactory logic, and the ERC1967 proxy initialized with
(admin, operator, impl721, impl1155).
- script/UpgradeCollectionFactory.s.sol: three-mode upgrade script with
ACTION env var: UPGRADE_FACTORY (UUPS upgradeToAndCall, with optional
REINIT_DATA), SET_IMPL_721, or SET_IMPL_1155 (admin pointer swap for
future clones only). Includes the full §9.4 pre-upgrade checklist in
the natspec.
- ops/deploy_collection_factory_zksync.sh: orchestration script that
loads .env-test/.env-prod, runs preflight checks, builds with --zksync,
invokes the Forge script, performs cast-based post-deploy sanity
checks (admin/operator role grants, impl pointers, EIP-1967 slot),
delegates source verification to verify_zksync_contracts.py, and
appends the deployed addresses to the env file.
- src/collections/layouts/{CollectionFactory,UserCollection721,
UserCollection1155}.v1.json: storage-layout baselines committed for
the §9.4 baseline-JSON convention. Verifies that lock booleans pack
into one slot (slot 3 offset 0/1 in 721; slot 1 offset 0/1 in 1155)
and that the gap is sized correctly (46 in 721, 47 in 1155 and the
factory).
- .cspell.json: allow codehash, codehashes, immediates, newbase,
newcontract, opping (referenced in test fixtures and contracts).
- spec §10: file layout extended to list all three layout baselines.
Required env vars per spec §9.1: N_FACTORY_ADMIN (multisig holding
DEFAULT_ADMIN_ROLE) and N_FACTORY_OPERATOR (backend service holding
OPERATOR_ROLE).
…clone() Records the spec-level decision and detailed design for swapping per-collection EIP-1167 minimal proxies for canonical OZ ERC1967Proxy with salt=externalId, unblocking zkSync Era deployment. Preserves the per-collection bytecode immutability promise (impls do not inherit UUPSUpgradeable, no ProxyAdmin) and keeps all v1 storage-layout baselines unchanged. Captures spec deltas, test impact, and audit checklist additions for the implementation phase.
…f Clones 13 task plan covering: 721 + 1155 factory refactor with TDD CREATE2-derivation tests, atomic-emits assertions, test-helper switch from Clones to ERC1967Proxy, no-upgrade-selector tests on impls, ERC1967Proxy bytecode-permanence test, integration test sequence-of-events updates, vocabulary pass, spec doc deltas, factoryDependencies CI gate, post-broadcast createCollection smoke test, and final storage-layout baseline verification.
forge build --zksync compiles the entire tree, so SwarmRegistryL1Upgradeable and test/upgrade-demo/TestUpgradeOnAnvil (both use EXTCODECOPY/SSTORE2) need to be moved out of the way before zksolc, then restored on EXIT. Mirrors the move/restore pattern in ops/deploy_swarm_contracts_zksync.sh. Required to unblock zkSync Sepolia dry-run for collections.
…createCollection721 Atomic deploy + init via the proxy's constructor (delegatecall to initialize). Salt = externalId gives off-chain CREATE2 pre-derivation. Preserves the per-collection bytecode-immutability promise — impls do not inherit UUPSUpgradeable, so the EIP-1967 impl slot is constructor- fixed. Required for zkSync Era compatibility (Clones.clone() is not supported on EraVM).
…createCollection1155 Symmetrical 1155 refactor following Task 2's 721 path. Drops the now- unused Clones import; both standards now deploy per-collection ERC1967Proxy via salted CREATE2.
Vocabulary pass clones to collections, deployment-model rewrite, §1.4 upgradeability footnote, §3.4 atomic-flow bullet, §4.1 sequence diagram, §4.4 gas profile, §4.5 new Address Determinism sub-section, §6.2 storage section, §7.2 row 15 split into 15a/15b + new row 16 for OZ proxy import audit posture, §9.1 deploy-script note, §9.4 per-collection EIP-1967 slot check.
Drops stale clone references in UserCollection*.sol, ICollectionFactory.sol, IUserCollection*.sol, CollectionTypes.sol, and doc/README.md NatSpec/comments. Identified as a plan gap by the Task 9 spec-compliance reviewer; the per-collection ERC1967Proxy architecture should be reflected in source-level documentation, not just the spec doc and tests.
CollectionFactory's factoryDependencies must contain at least one entry (ERC1967Proxy). Empty factoryDeps means the factory cannot deploy per-collection proxies on EraVM at runtime — exactly the failure mode that left the original Clones.clone() factory broken on zkSync Era.
Per Task 11 code-review feedback: capture jq's exit status explicitly so a malformed/truncated artifact JSON surfaces a curated log_error instead of '[: : integer expression expected'.
…Sync Calls createCollection721 against the freshly deployed factory and asserts the resulting collection has non-empty code and a populated EIP-1967 impl slot. Catches EraVM-runtime failures that compile-time gates miss (e.g. the original Clones.clone() incompatibility).
Per Task 12 code-review feedback (two HIGH-severity issues): 1. Pass the CreateParams721 tuple literal directly to `cast send` instead of pre-encoding via `cast abi-encode` (which produced a hex blob that `cast send`'s tuple parser would reject). 2. Resolve a signer key that actually holds OPERATOR_ROLE: prefer \$OPERATOR_PRIVATE_KEY, fall back to \$DEPLOYER_PRIVATE_KEY only when it matches \$N_FACTORY_OPERATOR, else skip with a clear warning. Production typically separates the deployer EOA from the operator, so the previous unconditional sign-with-deployer would always AccessControlUnauthorizedAccount-revert in prod.
Two header comments described the implementations as 'cloned per ERC-721/1155 collection' — stale post-refactor. The implementations are deployed once and shared across all per-collection ERC1967Proxy instances. Final stragglers from the Task 13 verification sweep. The L220 comment referencing the original Clones.clone() bug is intentional historical context and stays.
The smoke test sent the tuple as (string,string,address,address[],string,address,uint96,string) but the actual CreateParams721 struct in src/collections/interfaces/CollectionTypes.sol is (address owner, string name, string symbol, string baseURI, string contractURI, address royaltyRecipient, uint96 royaltyBps, address[] additionalMinters). The selector mismatch caused the call to revert with empty data. Verified post-fix on zkSync Sepolia: smoke test now successfully creates a per- collection ERC1967Proxy with all expected invariants (Upgraded → Initialized → RoleGranted → CollectionCreated emit order; EIP-1967 impl slot constructor-fixed; operator auto-granted MINTER_ROLE).
…-spec # Conflicts: # .cspell.json
Findings from a focused security/correctness review (operator-triggered collection factory). OZ behavior left unmodified throughout. F1 (metadata): correct the false "per-token URI anchored post-mint" claim. With a non-empty baseURI, OZ resolves tokenURI = baseURI + suffix and base is mutable until lockMetadata. Adopt option (b): callers pass relative suffixes; fix the double-prefix test; add a repoint-demonstrating test; update spec §7.2 row 7 / §3.5 and contract NatSpec. F2 (royalty observability): add DefaultRoyaltyUpdated event (ERC-2981 is event-less) on both impls + interfaces; assert it in tests. F3 (permanence on EraVM): the EVM opcode-walker does not cover the zksolc artifact; add verify_implementation_permanence ABI gate to the deploy script (no upgradeTo*/proxiableUUID selectors); document EVM-vs-EraVM. F4 (role finality): document the intentional no-DEFAULT_ADMIN_ROLE design and OWNER_ROLE non-transferability (spec §2.4 + NatSpec). F5 (mintBatch reentrancy): reserve the 721 ID range before the _safeMint loop so a reentrant mint takes a fresh ID; add regression test. F6 (factory invariant): pin the "initialize makes no external calls" requirement that keeps the registry-write-after-deploy reentrancy-safe. Also lands previously-uncommitted unit-test gap coverage (initialize guard branches, cross-standard externalId collision, 1155 onlyOperator, mintBatch boundary + empty cases). 85/85 collections tests pass; forge build + lint + spellcheck clean.
Deploy-script review findings (ops/deploy_collection_factory_zksync.sh): - H1: set -eo pipefail so a failed `forge ... | tee` aborts instead of being masked by tee's exit code. - H2: verify_deployment now ASSERTS instead of just logging — admin/operator hasRole must be true, impl pointers must match the deployed addresses, and the EIP-1967 slot must equal the factory logic (each exits non-zero on mismatch). Add a _norm_addr helper to compare padded slot words to addresses. - M1: smoke test asserts the collection's EIP-1967 slot equals the expected 721 impl (was logged but unchecked). - M3: interactive YES confirmation before any mainnet broadcast (bypass with CONFIRM_MAINNET=YES for CI). - M4: skip the smoke test on mainnet by default (it creates a permanent collection); opt in with RUN_MAINNET_SMOKE_TEST=true. - L1/L2/L3: document optional env vars, drop a dead `cast keccak ""` line, make verifier compiler/zksolc versions overridable. Verifier wiring (ops/verify_zksync_contracts.py): the deploy script calls this for source verification, but it had no mapping for the collections contracts — it would resolve 0 contracts, verify nothing, and report success. Fix: - Add CollectionFactory / UserCollection721 / UserCollection1155 to CONTRACT_SOURCE_MAP and a DeployCollectionFactoryZkSync.s.sol deploy-order sequence to BROADCAST_CONTRACT_SEQUENCE. - Fail loudly (exit 2) when a broadcast maps to zero verifiable contracts. - Guard the python call in verify_source_code with `|| exit_code=$?` so verification failure stays non-fatal under set -e instead of aborting. Upgrade script docs (script/UpgradeCollectionFactory.s.sol): - M2: usage examples now include the required --zksync (+ --slow) and document the L1-file move/restore compile prerequisite. bash -n, python py_compile, and a synthetic broadcast-parse check all pass.
ops/upgrade_collection_factory_zksync.sh drives UpgradeCollectionFactory.s.sol through the same safety scaffolding the deploy uses, for all three actions (UPGRADE_FACTORY / SET_IMPL_721 / SET_IMPL_1155): - L1-file move/restore + --zksync compile - action-specific artifact gate (factoryDeps for the factory; no upgrade selectors for collection impls) - pre-upgrade storage-layout diff vs the committed baseline (refuses to broadcast a non-append-only change without LAYOUT_REVIEWED=YES / interactive ack) - admin-key pre-check, mainnet confirmation guard - post-broadcast asserts (EIP-1967 slot / role / impl-pointer preservation) - single-contract source verification of the new impl Docs: point the Forge script NatSpec and spec §9.4/§10 at the wrapper. Validated: bash -n, action validation, _norm_addr, and the storage-layout diff logic (match + drift) all checked.
hardhat-deploy/DeployCollectionFactory.ts mirrors the Envelope/Swarm hardhat
scripts: deploys UserCollection721 + UserCollection1155 impls, CollectionFactory
logic, and the factory's ERC1967Proxy (atomic initialize), then verifies via
hre.run("verify:verify").
Motivation: the standard-JSON helper (ops/verify_zksync_contracts.py) cannot
verify CollectionFactory because it carries factoryDependencies (the ERC1967Proxy
bytecode hash) which that flow doesn't convey. The hardhat-zksync-verify plugin
handles factoryDeps, so the factory logic + both impls verify cleanly.
Notes baked in:
- ERC1967Proxy loaded/verified by fully-qualified @openzeppelin/... name (the
upgradable plugin ships a second ERC1967Proxy; the short name is ambiguous).
- Requires N_FACTORY_ADMIN / N_FACTORY_OPERATOR (same vars as the Foundry flow).
| // Admin | ||
| // ────────────────────────────────────────────── | ||
|
|
||
| function setImplementation721(address impl) external; |
There was a problem hiding this comment.
Because this interface is not aware of the contract having an admin or not (or in other words this interface could be used by contracts with or without owners adding the function requirements for only only contracts here may not be the best.
| // Initialization | ||
| // ────────────────────────────────────────────── | ||
|
|
||
| function initialize( |
There was a problem hiding this comment.
CollectionFactory is initializable so it must have the initialize function, you do not need to force it here.
Specifies an operator-triggered NFT collection factory: users pay in fiat off-chain, a trusted backend deploys a fully-isolated EIP-1167 clone (ERC-721 or ERC-1155) on the user's behalf. Factory is UUPS-upgradeable; clones are immutable per release. Both creator and operator hold MINTER_ROLE on each clone; per-collection metadata and royalties are owner-mutable until locked one-way. On-chain externalId map prevents duplicate creations from the off-chain ledger.